Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Review MySQL default null and nullable column definition #14974

Merged
merged 9 commits into from
May 2, 2020
Merged

Review MySQL default null and nullable column definition #14974

merged 9 commits into from
May 2, 2020

Conversation

Jeckerson
Copy link
Member

@Jeckerson Jeckerson commented May 2, 2020 •
edited
Loading

Hello!

In raising this pull request, I confirm the following:

  • I have read and understood the Contributing Guidelines
  • I have checked that another pull request for this purpose does not exist
  • I wrote some tests for this PR
  • I have updated the relevant CHANGELOG
  • I have created a PR for the documentation about this change

Current PR fixes previous PRs (#14888) behavior, when it wasn't possible to specify NULL in column definition, as some of types requires it, if column need to be nullable.
For example:
This is NOT NULL

updated_at TIMESTAMP

This is NULL, with default NULL

updated_at TIMESTAMP NULL

This is also NULL, with default NULL

updated_at TIMESTAMP NULL DEFAULT NULL

Two last examples was not possible to generate before this PR.

Also, moved NULL/NOT NULL before DEFAULT.

Before:

updated_at` TIMESTAMP DEFAULT NULL NULL

After

updated_at` TIMESTAMP NULL DEFAULT NULL

Previous branch - #14973

Thanks

@Jeckerson Jeckerson added bug A bug report 4.0.6 labels May 2, 2020
@Jeckerson Jeckerson self-assigned this May 2, 2020
sergeyklay
sergeyklay suggested changes May 2, 2020
View reviewed changes
Copy link
Contributor

@sergeyklay sergeyklay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some questions, probably due to the fact that the changes are not quite clear to me. There is no issue link, nor PR description

tests/integration/Db/Dialect/Mysql/ModifyColumnCest.php Outdated Show resolved Hide resolved
CHANGELOG-4.0.md Outdated Show resolved Hide resolved
tests/integration/Db/Dialect/Mysql/AddColumnCest.php Outdated Show resolved Hide resolved
tests/integration/Db/Dialect/Mysql/ModifyColumnCest.php Show resolved Hide resolved
@Jeckerson
Copy link
Member Author

Added link of previous PR (#14888) in the description.

@Jeckerson
Copy link
Member Author

Updated explanation into description.

sergeyklay
sergeyklay approved these changes May 2, 2020
View reviewed changes
Copy link
Contributor

@sergeyklay sergeyklay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@sergeyklay sergeyklay merged commit 923c378 into 4.0.x May 2, 2020
@sergeyklay sergeyklay deleted the mysql-default-null branch May 2, 2020 11:19
@sergeyklay
Copy link
Contributor

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sergeyklay sergeyklay sergeyklay approved these changes

@niden niden Awaiting requested review from niden

@SidRoberts SidRoberts Awaiting requested review from SidRoberts

Assignees

@Jeckerson Jeckerson

Labels
bug A bug report
Projects
Phalcon v5
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Jeckerson @sergeyklay